Reviewed 2 of 3 files at r8.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

crates/blockifier/src/transaction/transactions_test.rs line 912 at r4 (raw file):

Nope, it should be max amount (balance >= price * amount)

crates/blockifier/src/transaction/transactions_test.rs line 961 at r4 (raw file):

Why can't we just use the maxes instead of the mins (and then avoid defining them)? I assume we fail on the per resource assertion before the total balance assertion.

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

The bug? Do you mean that min_amount could have been larger than the max_amount and now it doesn't? I don't understand what happened

crates/blockifier/src/transaction/transactions_test.rs line 971 at r8 (raw file):

    let min_l2_amount = 445500;
    let min_l1_data_amount = 128;
    let max_l2_gas_price: u64 =

Suggestion:

max_l2_gas_amount

crates/blockifier/src/transaction/transactions_test.rs line 973 at r8 (raw file):

    let max_l2_gas_price: u64 =
        (BALANCE / MAX_L2_GAS_PRICE).try_into().expect("Failed to convert u128 to u64.");
    let max_l1_data_gas_price: u64 =

Suggestion:

max_l1_data_gas_amount

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

crates/blockifier/src/transaction/transactions_test.rs line 912 at r4 (raw file):

Done. Sorry, I must have been super tired 😬

crates/blockifier/src/transaction/transactions_test.rs line 961 at r4 (raw file):

1. It wouldn't help if min_amount>`max_amount' (as was the case before the bug-fix)
2. It's better to make the test as tight as possible; I'm adding the min_amount only because it's necessary, putting max_amount slightly defeats the point of the test.

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

There was a bug that made the l2_gas_price super high, so the max_amount was very low (in particular, lower than min_amount); now that it's back to normal, there's no reason min_amount should be bigger than max_amount (though, technically, it can happen...). Your call if you want me to insert max(...).

crates/blockifier/src/transaction/transactions_test.rs line 971 at r8 (raw file):

    let min_l2_amount = 445500;
    let min_l1_data_amount = 128;
    let max_l2_gas_price: u64 =

Done.

crates/blockifier/src/transaction/transactions_test.rs line 973 at r8 (raw file):

    let max_l2_gas_price: u64 =
        (BALANCE / MAX_L2_GAS_PRICE).try_into().expect("Failed to convert u128 to u64.");
    let max_l1_data_gas_price: u64 =

Done.

Reviewed 1 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

crates/blockifier/src/transaction/transactions_test.rs line 968 at r4 (raw file):

Never mind, too much of a change, I'll try to open a PR later on.

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

Replace with asserts that max_amount/3 >= min_amount

crates/blockifier/src/transaction/transactions_test.rs line 964 at r8 (raw file):

        class_info,
    );
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

Suggestion:

    // V3 invoke.
    
    for (l1_max_amount, l2_max_amount, l1_data_max_amount) in [
        (max_l1_gas_price + 1, min_l2_amount, min_l1_data_amount),
        (min_l1_amount, max_l2_gas_price + 1, min_l1_data_amount),
        (min_l1_amount, min_l2_amount, max_l1_data_gas_price + 1),
        (max_l1_gas_price / 3 + 1, max_l2_gas_price / 3 + 1, max_l1_data_gas_price / 3 + 1),
    ] {
        let invalid_resource_bounds = ValidResourceBounds::AllResources(AllResourceBounds {
            l1_gas: ResourceBounds {
                max_amount: l1_max_amount,
                max_price_per_unit: MAX_L1_GAS_PRICE,
            },
            l2_gas: ResourceBounds {
                max_amount: l2_max_amount,
                max_price_per_unit: MAX_L2_GAS_PRICE,
            },
            l1_data_gas: ResourceBounds {
                max_amount: l1_data_max_amount,
                max_price_per_unit: MAX_L1_DATA_GAS_PRICE,
            },
        });

    let invalid_tx = account_invoke_tx(invoke_tx_args! {
        resource_bounds: invalid_l1_resource_bounds,
        ..default_args.clone()
    });
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

    // Deploy.
    let invalid_tx = AccountTransaction::DeployAccount(deploy_account_tx(
        deploy_account_tx_args! {
            resource_bounds: max_l1_resource_bounds,
            class_hash: test_contract.get_class_hash()
        },
        &mut NonceManager::default(),
    ));
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

    // Declare.
    let contract_to_declare = FeatureContract::Empty(CairoVersion::Cairo1);
    let class_info = calculate_class_info_for_testing(contract_to_declare.get_class());
    let invalid_tx = declare_tx(
        declare_tx_args! {
            class_hash: contract_to_declare.get_class_hash(),
            compiled_class_hash: contract_to_declare.get_compiled_class_hash(),
            sender_address: account_contract_address,
            resource_bounds: invalid_l1_resource_bounds,
        },
        class_info,
    );
    assert_failure_if_resource_bounds_exceed_balance(state, block_context, invalid_tx);

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

crates/blockifier/src/transaction/transactions_test.rs line 968 at r4 (raw file):

FYI https://reviewable.io/reviews/starkware-libs/sequencer/1228

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

crates/blockifier/src/transaction/transactions_test.rs line 969 at r4 (raw file):

Done.

Reviewed 1 of 3 files at r12.
Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @nimrod-starkware)

Reviewed 1 of 4 files at r13, 3 of 3 files at r15, all commit messages.
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)

crates/blockifier/src/test_utils.rs line 105 at r15 (raw file):

pub const MAX_L1_GAS_PRICE: NonzeroGasPrice = DEFAULT_STRK_L1_GAS_PRICE;
pub const MAX_RESOURCE_COMMITMENT: Fee =
    DEFAULT_L1_GAS_AMOUNT.nonzero_saturating_mul(MAX_L1_GAS_PRICE);

Not used

Code quote:

pub const MAX_RESOURCE_COMMITMENT: Fee =
    DEFAULT_L1_GAS_AMOUNT.nonzero_saturating_mul(MAX_L1_GAS_PRICE);

crates/blockifier/src/transaction/transactions_test.rs line 937 at r15 (raw file):

    #[values(CairoVersion::Cairo0, CairoVersion::Cairo1)] account_cairo_version: CairoVersion,
) {
    use starknet_api::execution_resources::GasAmount;

move up

crates/starknet_api/src/execution_resources.rs line 28 at r15 (raw file):

    Hash,
)]
#[cfg_attr(any(test, feature = "testing"), derive(derive_more::Div))]

Suggestion:

#[cfg_attr(
    any(test, feature = "testing"),
    derive(derive_more::Add, derive_more::Div, derive_more::Sum, derive_more::AddAssign)
)]
#[derive(
    derive_more::Display,
    Clone,
    Copy,
    Debug,
    Default,
    Eq,
    PartialEq,
    PartialOrd,
    Ord,
    Serialize,
    Deserialize,
    Hash,
)]

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)

crates/blockifier/src/test_utils.rs line 105 at r15 (raw file):

Done.

crates/blockifier/src/transaction/transactions_test.rs line 937 at r15 (raw file):

Done.

crates/starknet_api/src/execution_resources.rs line 28 at r15 (raw file):

    Hash,
)]
#[cfg_attr(any(test, feature = "testing"), derive(derive_more::Div))]

Done.

Reviewed 3 of 3 files at r16, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @nimrod-starkware)

Reviewed 1 of 3 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)